Fix #78 - Implement server side pagination and sorting for queries #2566
Conversation
…egrated solution.
| itemsPerPage = 20, | ||
| orderByField, | ||
| orderByReverse = false, | ||
| params = {}, |
| itemsPerPage: this.pageSize, | ||
| orderByField: pageOrder, | ||
| orderByReverse: pageOrderReverse, | ||
| params: this.parameters, |
There was a problem hiding this comment.
does this.parameters have a value at this point? (I see where update() assigns to it later.)
There was a problem hiding this comment.
It's a bit of a kerfuffle, I'll clean this up while porting this to #2573
| this.searchTerm = ''; | ||
| } | ||
| this.paginator.itemsPerPage = this.pageSize; | ||
| this.paginator.params = this.parameters(); |
There was a problem hiding this comment.
any reason not to just inline the parameters function? I found the similar name to params confusing.
|
@arikfr This is the PR I talked about recently, please let me know if you want me to explain the implementation details if needed. |
| @@ -1,12 +1,23 @@ | |||
| import moment from 'moment'; | |||
| import { isString } from 'underscore'; | |||
| import startsWith from 'underscore.string/startsWith'; | |||
There was a problem hiding this comment.
You can use String's own startsWith.
There was a problem hiding this comment.
Hm, I thought it's not supported by IE11?
There was a problem hiding this comment.
Thinking about IE11 is very benevolent of you :) But I would assume Babel takes care of this for us?
| @@ -1,12 +1,23 @@ | |||
| import moment from 'moment'; | |||
| import { isString } from 'underscore'; | |||
There was a problem hiding this comment.
If the React pull request gets merged before this one, we need to remember to replace this with lodash...
| order = request.args.get('order', '').strip() or default_order | ||
| # and if it matches a long-form for related fields, falling | ||
| # back to the default order | ||
| selected_order = order_map.get(order, order) |
There was a problem hiding this comment.
Maybe we should use default_order as the default value here as well, just to make sure that we only get predefined values for ordering?
| selected_order = order_map.get(order, order) | ||
| # The query may already have an ORDER BY statement attached | ||
| # so we clear it here and apply the selected order | ||
| return sort_query(results.order_by(None), selected_order) |
There was a problem hiding this comment.
Interesting. What would happen without this?
There was a problem hiding this comment.
By default, the all_queries class method sorts by created_at, which means we just reset it here to be able to apply the sorting according to the query parameter.
There was a problem hiding this comment.
But isn't SQLA smart enough to just replace the order_by when called again?
| search=term, | ||
| drafts='true' if include_drafts else 'false' | ||
| ) | ||
| return {}, 301, {'Location': new_location} |
There was a problem hiding this comment.
We always deploy API and client side together, so maybe we can just remove this?
There was a problem hiding this comment.
Ah, that'd be nicer indeed. And you don't make any guarantees regarding the API stability?
There was a problem hiding this comment.
Not really 🙄
I wonder if there are people who are using this API endpoint not in the application? 🤔
| .options( | ||
| contains_eager(Query.user), | ||
| contains_eager(Query.latest_query_data), | ||
| ) |
There was a problem hiding this comment.
Yeah, SQA generated different queries otherwise 😬
This was initially reported in #78 but also applies to the queries search where the list of results can become quite large depending on the number of queries and search term used.
This was originally reported in mozilla#384 and mozilla#385 and also fixes also mozilla#323 (which was not reported here initially).
It deprecates the standalone backend query search handler and client query search resource and implements redirects for those.
I believe the live paginator should also be used in more list views and the non-live paginator deprecated or outright removed. I'm not sure how Redash is handling API backward compatibility, so I erred on the side of caution and followed a deprecation policy (that could mean removing old APIs after the next version for example).